Return tuple for aiohttp #849

Jyhess · 2019-01-14T09:49:26Z

First step of #828

Changes proposed in this pull request:

Support tuples as return values for aiohttp handlers

ainquel · 2019-01-16T08:41:45Z

Hi, I just reviewed your code, firsly thanks to help on this one ! :)
I don't know what @dtkav thinks about it maybe he will merge it but this is not going in the way I want in #828 .
To explain myself, what I want in connexion is as little as possible work to do when you create a new App.
For me validation of handler response is not something specific to FlaskApp or AioHttpApp. It has to be handled by one central and well tested piece of code, shared by all Apps to ensure consistency. This is why I've done it here.
Then App._response_from_handler is here to make some boilerplate before validating the handler's response and to create a Framework specific response from the data returned by this validation.

What I explained juste before applies also for the tests. In my PR, I want same tests to be applied for each App, to not create duplication. In my point a view, This project needs a base of tests that can be run on all tests and very specific App tests (like using a custom encoder in Flask by passing it to the flask object)

Here is how I see this functionality ! :)

Jyhess · 2019-01-16T15:22:24Z

@panpan I got your point. I did more factorization between both Flask and AioHttp. Let me know what you think.
I let you working on test side ;)

dtkav · 2019-01-22T00:32:57Z

@Jyhess I looks like the CI is failing because your are trying to read the response body (bytes) as a string. I think you can use response.json instead.

cognifloyd · 2019-04-17T14:41:17Z

I would like to use tuple returns with aiohttp. What is needed to move this forward? More code review?

jmcs · 2019-05-03T06:58:54Z

@cognifloyd sorry for dropping the ball here. Can you fix the merge conflicts?

cognifloyd · 2019-05-03T18:32:35Z

Sure. Here's the merge commit:
cognifloyd@b4b72e7
I don't have commit rights in the connexion repo, so I can't update this PR.
git mergetool was able to auto-resolve the differences in the two files.

Jyhess · 2019-05-09T15:40:12Z

I rebased the branch on top of master.
Is there something else I can do to move further on this pull request?

cognifloyd

I tested this in the connexion-aiohttp project I'm working on.
Before trying this, I was always returning a ConnexionResponse.
With this change, I successfully returned each of these:

ConnexionResponse,
a two-tuple with body, 200
aiohttp.web.Response(body=json.dumps(body), content_type="application/json")

This works for me. 🎉
edited: I hit enter too soon.

connexion/apis/abstract.py

        :param response: A response to cast.
+        :param mimetype: The response mimetype.
+        :param url: The url to wrote in logs


hjacobs · 2019-10-15T16:42:48Z

Could you rebase the PR and fix the conflicts? We will take a look afterwards.

dtkav · 2019-11-10T18:19:28Z

Hi @Jyhess - I tried to resolve the conflicts against master. Sorry if this messed up your workflow!

cognifloyd

✔️

cognifloyd

Some questions while I diagnose what is causing the test failure.

connexion/apis/flask_api.py

+    @classmethod
+    def _jsonify_data(cls, data, mimetype):
+        # TODO: to discuss: Using jsonifier for all type of data, even when mimetype is not json is strange. Why ?
+        if (isinstance(mimetype, six.string_types) and is_json_mimetype(mimetype)) \
+                or not (isinstance(data, six.binary_type) or isinstance(data, six.text_type)):
+            return cls.jsonifier.dumps(data)
+
+        return data


cognifloyd · 2019-12-04T18:05:52Z

connexion/apis/aiohttp_api.py

-                         response.status, extra={'data': response.body, 'url': url})
+        # TODO: I let this log here for full compatibility. But if we can modify it, it can go to _get_response()
+        logger.debug('Got stream response with status code (%d)',
+                     response.status, extra={'url': url})


connexion/apis/abstract.py

+        if isinstance(response, tuple):
+            len_response = len(response)
+            if len_response == 2:
+                if isinstance(response[1], (int, Enum)):
+                    data, status_code = response
+                    return cls._build_response(mimetype=mimetype, data=data, status_code=status_code)
+                else:
+                    data, headers = response
+                return cls._build_response(mimetype=mimetype, data=data, headers=headers)
+            elif len_response == 3:
+                data, status_code, headers = response
+                return cls._build_response(mimetype=mimetype, data=data, status_code=status_code, headers=headers)
+            else:
+                raise TypeError(
+                    'The view function did not return a valid response tuple.'
+                    ' The tuple must have the form (body), (body, status, headers),'
+                    ' (body, status), or (body, headers).'
+                )


cognifloyd

notes about six and python3

connexion/apis/abstract.py

+        if isinstance(response, ConnexionResponse):
+            # If body in ConnexionResponse is not byte, it may not pass schema validation.
+            # In this case, rebuild response with aiohttp to have consistency
+            if response.body is None or isinstance(response.body, six.binary_type):


connexion/apis/abstract.py

+        if not isinstance(data, six.binary_type):
+            if isinstance(mimetype, six.string_types) and is_json_mimetype(mimetype):
+                body = cls.jsonifier.dumps(data)
+            elif isinstance(data, six.text_type):


cognifloyd · 2019-12-03T05:13:20Z

@Jyhess I hope we can work on this PR next. Are you available to help resolve some of my questions?

Jyhess · 2019-12-03T10:06:52Z

@cognifloyd I will ping you on gitter when I can go back on this subject. Maybe this afternoon or tomorrow.

connexion/apis/abstract.py

dtkav · 2019-12-04T19:58:47Z

This is fantastic work @Jyhess and @cognifloyd - I'm inspired by your work on this project, and excited to feel the momentum!

It looks like the longer-term plan is to firm up this interface to allow for other frameworks (like quart, sanic).
I think it's worth discussing a few high level things (not blocking this PR)

Designing an internal API like this is hard, I think it could be valuable to at least demo a implementation of a third framework with this new interface. Proving that it would work with 3 frameworks is a good sign that it will work for many. Making it work for two doesn't force you to generalize as much (which I think is what led to the current design).
It was pretty high cost to support both aiohttp and flask when I introduced openapi3 support. What are some approaches for unifying the test infrastructure, and supporting the sync/async differences more generally?

I'd be excited to collaborate on long term vision and improvements assuming there's bandwidth for review and merging by folks at zalando.

cognifloyd

@hjacobs I think this is ready to merge. We can skip removing _jsonify_data from flask_api for now.

dtkav · 2019-12-04T20:51:27Z

tests/aiohttp/test_get_response.py

+    response = yield from api.get_response({'foo': 'bar'})
+    assert isinstance(response, web.Response)
+    assert response.status == 200
+    assert response.body == b"{'foo': 'bar'}"


You wouldn't expect this to be json?

Right. I would not expect this to be json. Train of thought follows:

Everything in this file is testing against an api that has only one endpoint /greeting/{name} -> fakeapi.aiohttp_handlers.aiohttp_post_greeting which returns a dict.

But it's not actually using the handler, it's accessing get_response directly.

So mimetype is not coming from the spec here, which means there is no assumption that the content will be json.

which means if you pass an object (in this case a dict) it will be stringified by _jsonify_data.

the implementation of _jsonify_data in abstract is basically doing the same thing that _cast_body did in aiohttp_api

Note that this body is using ' not " so it is not valid json. it is just a stringified dict.

So, yes I would expect (without changing behavior) this to merely stringify the dict as no mimetype / content type is defined.

Consider the next test where a mimetype is explicitly defined, and therefore produces json data.

I think I can follow your logic here - I have a minor pragmatic hangup which is that casting a dict to a string is kind of the least-useful way to serialize that object.
You might argue in practice that folks would notice this, and then include the right content-type, but it seems like an unfriendly default.
Consider the following flask app, which defaults to returning json

import flask app = flask.Flask(__name__) @app.route("/") def a(): return {"a": "b"} app.run()

Does that mean in that example you'd expect to get a string representation of the dict as text/plain?

cognifloyd · 2019-12-06T00:18:06Z

K. So my change is a little more restrictive than flask's _jsonify_data, but hopefully it will be less surprising when people are trying to serialize with something other than json.
If the mimetype is provided and it is not json, then it will only run data through str() even if it is a dict or anything like that.

But, this doesn't automatically change the mimetype as there is no obvious place to "change" a requested mimetype/content_type.

I suppose it could be done just for aiohttp in the aiohttp-specific version of get_response, but that feels like a weird place to start introspecting the data to change the content_type. If we do want to do that in get_response instead of in _jsonify_data, then we could rely on is_json_mimetype to trigger the jsonification in _jsonify_data instead of inspecting the data type there. But if we start introspecting the data type in get_response, does that mean that every other backend is going to do that as well? Where is flask magically turning this into application/json?

dtkav · 2019-12-06T01:59:39Z

connexion/apis/abstract.py

+            elif isinstance(data, str):
+                body = data
+            else:
+                body = str(data)


What are some other datatypes where the string representation is better? I'm having trouble thinking of any.
For both int and float, I think I'd prefer the json representation.

ok. Now, when mimetype is None, anything that can be jsonified will be, catching TypeError to rescue with str(). Duck-typing for the win - if it quacks like JSON and the mimetype is None, then it is JSON.

I think this is getting really close.
Eventually we want to be able to register serializers for arbitrary mimetypes, so I think there's a default path (mimetype is none, try to do json), and then maybe some built-in overridable serializers (text/plain uses str() for example).
So if we can get backwards compatible behavior with flask (default serializer is json), but an escape hatch registering new mimetypes/serializers then I think we're golden.
One thing I don't think we should do is introduce anything opinionated, or anything too magical/surprising.

It turns out that it is magical and surprising no matter how I look at it. From both aiohttp and flask sides, it feels a little off. :(

cognifloyd · 2019-12-07T01:00:36Z

OK. So, harmonizing the serialization between aiohttp and flask turns out to be ugly.

Several of the flask tests specify a mimetype of text/plain and then expect the response to be jsonified. So, I had to do if mimetype in [None, "text/plain"]. Validators also allow for validating json in both application/json and text/plain.

From the tests it looks like you can currently return data with aiohttp, but you can't return a tuple. Data may be any type and then it would be serialized in _cast_body to some kind of string. So, we can change the aiohttp serialization if something is included in a tuple, but returning a dict with mimetype text/plain (or no mimetype) currently just serializes to a dict literal which is not valid JSON.

I also had to adjust one of those aiohttp tests because now that it is jsonified instead of just stringified, there's a newline at the end.

So, this is cleaner and more consistent, but harmonizing the jsonification might have to be a separate PR that goes in a major version bump. I can imagine this being part of a pluggable serializers feature. It wouldn't be too much more work to have a chain of serializers that vote based on the mediatype.

How far can we harmonize serialization in this PR?

cognifloyd · 2019-12-07T01:05:15Z

We could introduce the planned new behavior in abstract and then override it with the backwards compatible equivalent in both aiohttp_api and flask_api.

Rename _jsonify_data to _serialize_data to make its purpose easier to understand (this was also known as _cast_body in aiohttp_api). In exploring how to harmonize json serialization between aiothttp and flask, we needed to be able to adjust the mimetype from within _serialize_data. Harmonizing the actual serialization has to wait until backwards incompatible changes can be made, but we can keep the new interface, as these functions were introduced in this PR (spec-first#849).

cognifloyd · 2019-12-07T01:42:51Z

K. I removed the commits that harmonized serialization between flask and aiohttp. I will push a new PR where we can continue that work, but I expect that will have to wait until a major version bump so we can make backwards incompatible adjustments.

dtkav · 2019-12-07T01:47:42Z

ok that makes sense. Happy to tackle it in another PR.
What do you think about adding a deprecation warning for folks using weird defaults?
I'm feeling more and more excited about just fixing all of this internal api mismatch, and pluggable serialization and deserialization on a branch in prep for a major version bump.

cognifloyd · 2019-12-07T16:26:15Z

I added some FutureWarnings about deprecation where behavior will need to change to harmonize serialization.

warnings about deprecated features when those warnings are intended for end users of applications that are written in Python.

I picked FutureWarning because it seemed to fit better than DeprecationWarning

warnings about deprecated features when those warnings are intended for other Python developers (ignored by default, unless triggered by code in __main__).

cognifloyd · 2019-12-10T07:52:18Z

@dtkav @hjacobs @jmcs I think is ready to merge.

dtkav

Great work - Thanks @Jyhess and @cognifloyd

hjacobs · 2019-12-11T11:54:33Z

👍

Jyhess mentioned this pull request Jan 14, 2019

WIP: Run framework agnostic tests #828

Closed

Jyhess force-pushed the return_tuple_for_aiohttp branch from 584e659 to f5c3f2f Compare January 14, 2019 10:01

spec-first deleted a comment Jan 17, 2019

Jyhess force-pushed the return_tuple_for_aiohttp branch 4 times, most recently from d0cd874 to da810b7 Compare February 4, 2019 18:02

spec-first deleted a comment Feb 4, 2019

Jyhess force-pushed the return_tuple_for_aiohttp branch 2 times, most recently from f090f42 to b8cd718 Compare May 7, 2019 16:25

cognifloyd reviewed May 22, 2019

View reviewed changes

connexion/apis/abstract.py Outdated

:param response: A response to cast.

:param mimetype: The response mimetype.

:param url: The url to wrote in logs

This comment was marked as resolved.

Sign in to view

cognifloyd approved these changes May 22, 2019

View reviewed changes

cognifloyd approved these changes Nov 11, 2019

View reviewed changes

cognifloyd suggested changes Nov 11, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

cognifloyd reviewed Nov 12, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

Support aiohttp handlers to return tuples

789f1e9

dtkav reviewed Dec 4, 2019

View reviewed changes

connexion/apis/abstract.py Outdated Show resolved Hide resolved

clean up a couple of type hint comments

2ba9e9a

cognifloyd approved these changes Dec 4, 2019

View reviewed changes

cognifloyd mentioned this pull request Dec 4, 2019

Async security check #869

Merged

dtkav reviewed Dec 4, 2019

View reviewed changes

cognifloyd mentioned this pull request Dec 4, 2019

Naive switch from Flask to AIOHTTP fails #578

Closed

dtkav reviewed Dec 6, 2019

View reviewed changes

cognifloyd force-pushed the return_tuple_for_aiohttp branch 2 times, most recently from 3d5469f to 85eeceb Compare December 7, 2019 00:37

Add a few more get_response tests

f3e763e

cognifloyd force-pushed the return_tuple_for_aiohttp branch from 85eeceb to 3a33f74 Compare December 7, 2019 01:29

cognifloyd force-pushed the return_tuple_for_aiohttp branch from 3a33f74 to e57e6a6 Compare December 7, 2019 01:39

cognifloyd mentioned this pull request Dec 7, 2019

WIP Harmonize serialization between aiohttp and flask #1095

Closed

cognifloyd force-pushed the return_tuple_for_aiohttp branch from c1926a5 to f029a01 Compare December 7, 2019 16:03

Add deprecation warnings about implicit serialization

1275511

cognifloyd force-pushed the return_tuple_for_aiohttp branch from f029a01 to 1275511 Compare December 7, 2019 16:12

dtkav approved these changes Dec 10, 2019

View reviewed changes

cognifloyd mentioned this pull request Dec 11, 2019

Allow asynchronous routes #1012

Closed

hjacobs merged commit d18c387 into spec-first:master Dec 11, 2019

Return tuple for aiohttp #849

Return tuple for aiohttp #849

Conversation

Jyhess commented Jan 14, 2019

ainquel commented Jan 16, 2019

Jyhess commented Jan 16, 2019

dtkav commented Jan 22, 2019

cognifloyd commented Apr 17, 2019

jmcs commented May 3, 2019

cognifloyd commented May 3, 2019

Jyhess commented May 9, 2019

cognifloyd left a comment • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

hjacobs commented Oct 15, 2019

dtkav commented Nov 10, 2019

cognifloyd left a comment

Choose a reason for hiding this comment

cognifloyd left a comment • edited Loading

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment has been minimized.

cognifloyd left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment has been minimized.

cognifloyd commented Dec 3, 2019

Jyhess commented Dec 3, 2019

dtkav commented Dec 4, 2019

cognifloyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cognifloyd commented Dec 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cognifloyd commented Dec 7, 2019

cognifloyd commented Dec 7, 2019

cognifloyd commented Dec 7, 2019

dtkav commented Dec 7, 2019 • edited Loading

cognifloyd commented Dec 7, 2019 • edited Loading

cognifloyd commented Dec 10, 2019

dtkav left a comment

Choose a reason for hiding this comment

hjacobs commented Dec 11, 2019

cognifloyd left a comment •

edited

Loading

cognifloyd left a comment •

edited

Loading

cognifloyd commented Dec 6, 2019 •

edited

Loading

dtkav commented Dec 7, 2019 •

edited

Loading

cognifloyd commented Dec 7, 2019 •

edited

Loading